[SPARK-29930][SQL][FOLLOW-UP] Allow only default value to be set for removed SQL configs #27057
Conversation
|
Test build #115984 has finished for PR 27057 at commit
|
| * map values are tuples of default config value and the version in which | ||
| * the SQL config was removed. | ||
| */ | ||
| val removedSQLConfigs = Map[String, (String, String)]( |
There was a problem hiding this comment.
Oh, yeah. I was thinking about having this mechanism in SQL configurations too but couldn't find some time to fix.
There is an example to follow in SparkConf, for example, see deprecatedConfigs.
Can you match how it looks like? Then it should be good to go.
There was a problem hiding this comment.
Just to add more info, deprecatedConfigs works like removedSLQConfigs (because they are deprecated first and removed out).
|
cc @vanzin @cloud-fan @gatorsmile who I talked about this with |
|
|
||
| test("set removed configs to non-default values") { | ||
| Seq( | ||
| "spark.sql.fromJsonForceNullableSchema" -> false, |
There was a problem hiding this comment.
shall we only test one config? Otherwise we may need to update this test again and again when we remove more configs in the future.
In cannot set/unset static SQL conf we only test one config too.
There was a problem hiding this comment.
I agree. I will test only spark.sql.fromJsonForceNullableSchema
|
LGTM, nice feature! |
|
Test build #116030 has finished for PR 27057 at commit
|
|
Test build #116044 has finished for PR 27057 at commit
|
| "It was removed to prevent loosing of users data for non-default value."), | ||
| RemovedConfig("spark.sql.legacy.compareDateTimestampInTimestamp", "3.0.0", "true", | ||
| "It was removed to prevent errors like SPARK-23549 for non-default value.") | ||
| ) |
There was a problem hiding this comment.
Looks a nice feature. Just a question; any policy about when these entries will be removed in this list? We need to keep them forever, or we can remove them sometime?
There was a problem hiding this comment.
I think we can remove those after few minor releases or when we bump up the major release.
|
Merged to master. @MaxGekk are you also interested in grouping deprecated configurations too? Hope we can remove out legacy configurations out a bit more aggressively by leveraging those mechanism. |
yes, I am. |
|
Here is the PR #27092 |
6 participants
What changes were proposed in this pull request?
In the PR, I propose to throw
AnalysisExceptionwhen a removed SQL config is set to non-default value. The following SQL configs removed by #26559 are marked as removed:spark.sql.fromJsonForceNullableSchemaspark.sql.legacy.compareDateTimestampInTimestampspark.sql.legacy.allowCreatingManagedTableUsingNonemptyLocationWhy are the changes needed?
To improve user experience with Spark SQL by notifying of removed SQL configs used by users.
Does this PR introduce any user-facing change?
Yes, before the
setcommand was silently ignored:after the exception should be raised:
How was this patch tested?
Added new tests into
SQLConfSuitefor both cases when removed SQL configs are set to default and non-default values.